Skip to content

Fix/windows job object protection - #860

Open
ilyaxuwu wants to merge 5 commits into
CodebuffAI:mainfrom
ilyaxuwu:fix/windows-job-object-protection
Open

Fix/windows job object protection#860
ilyaxuwu wants to merge 5 commits into
CodebuffAI:mainfrom
ilyaxuwu:fix/windows-job-object-protection

Conversation

@ilyaxuwu

@ilyaxuwu ilyaxuwu commented Jul 10, 2026

Copy link
Copy Markdown

Job Object protection has been added to prevent child processes spawned in Windows from remaining active when the parent crashes.

isolated-command.ts — Using Bun.FFI, CreateJobObjectW, AssignProcessToJobObject, and SetInformationJobObject are called via kernel32.dll. Thanks to the JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE flag, Windows automatically cleans up the child process when the parent dies. If FFI cannot be found, it silently falls back.
run-terminal-command.ts — After spawning, protectChildWithJob(pid) is called, and cleanup is triggered on the process close event.

@codebuff-team

Copy link
Copy Markdown
Contributor

Good instinct — orphaned child processes on Windows when the parent crashes/gets taskkill /F'd is a real, annoying bug, and Job Objects with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE are the correct fix.

The implementation has a critical bug though. In isolated-command.ts, limitInfo is allocated as Buffer.alloc(48) and 48 is passed as the length to SetInformationJobObject. On 64-bit Windows, JOBOBJECT_BASIC_LIMIT_INFORMATION is actually 64 bytes (two LARGE_INTEGER fields, DWORD LimitFlags at offset 16, then padding + two 8-byte SIZE_T fields + DWORD ActiveProcessLimit + padding + 8-byte ULONG_PTR Affinity + two more DWORDs = 64 bytes total). SetInformationJobObject validates the buffer length against the expected size for the information class and will reject a 48-byte buffer with ERROR_INVALID_PARAMETER. Because the code treats that failure as "gracefully unsupported" (if (!limitOk) { CloseHandle; return null }), the feature will silently never activate on essentially all real 64-bit Windows machines — exactly the case this PR is trying to fix. Worth fixing the struct size (and probably testing against GetLastError() during development to confirm the call actually succeeds, since the current fallback path masks this kind of failure).

Other things a maintainer will want before porting:

  • No tests. This is exactly the kind of platform-specific, hand-packed-struct code that benefits from a mocked-FFI unit test asserting the buffer layout/size.
  • The MSYS disable_pcon env var change in run-terminal-command.ts is bundled into the same PR but is unrelated to Job Object protection — worth splitting out with its own justification/repro.
  • require('bun:ffi') assumes a Bun runtime; confirm that's a fair assumption everywhere runTerminalCommand is invoked.

Solid direction, but the core mechanism needs to actually work before it's portable.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants